Conversation
ViktorSalk
commented
Mar 24, 2025
- Создал ветку: add-controllers.
- Создал четыре пакета: item, booking, request и user.
- Создал DTO-объекты и мапперы для сущностей: ItemDto, UserDto, BookingDto, ItemRequestDto
- Создал тесты на новую функциональность.
1) Создал ветку: add-controllers. 2) Создал четыре пакета: item, booking, request и user. 3) Создал DTO-объекты и мапперы для сущностей: ItemDto, UserDto, BookingDto, ItemRequestDto 4) Создал тесты на новую функциональность.
|
|
||
|
|
||
| /** | ||
| * TODO Sprint add-controllers. |
There was a problem hiding this comment.
В коммерческой разработке, когда код из TODO реализован, принято его удалять, чтобы он не путал других разработчиков.
Удали, пожалуйста, все TODO, которые были выполнены во всем проекте
| @Override | ||
| public List<ItemDto> getAll(Long userId) { | ||
| List<ItemDto> items = new ArrayList<>(); | ||
| for (Item item : itemRepository.findAll()) { |
There was a problem hiding this comment.
В коммерческой разработке уже давно сложно встретить обычные циклы, так как в java 8 был представлен более мощный инструмент для работы с коллекциями - stream api. С помощью стримов и лямбда - выражений работать с коллекциями гораздо удобнее и нативно понятнее, поэтому важно уметь их применять в своем коде
There was a problem hiding this comment.
Использовал stream api в методе getAll().
| } | ||
|
|
||
| @Override | ||
| public ItemDto update(ItemDto itemDto, Long id, Long userId) { |
There was a problem hiding this comment.
Обновление полей лучше вынести в метод в маппере, так как он занимается перекладыванием полей из одного объекта в другой
There was a problem hiding this comment.
Вынес метод Обновление полей в маппер, после внедрения MapStruct, генерируется автоматически.
| if (text.isBlank()) { | ||
| return searchedItems; | ||
| } | ||
| for (Item item : itemRepository.findAll()) { |
There was a problem hiding this comment.
Улучшил код, используя Stream API.
| import ru.practicum.shareit.item.dto.ItemDto; | ||
| import ru.practicum.shareit.item.model.Item; | ||
|
|
||
| public class ItemMapper { |
There was a problem hiding this comment.
Саморазвитие
В коммерческой разработке принято максимально избавляться от шаблонного кода, чтобы не перегружать и так большие и объемные проекты. Для маппинга сущностей часто используется библиотека Mapstruct, которая позволяет автоматически настраивать маппинг из одной сущности в другую с помощью описания интерфейсов и аннотаций.
Вот здесь есть отличный гайд для старта работы с библиотекой.
При наличии свободного времени можно поизучать и попробовать прикрутить в проект
There was a problem hiding this comment.
Спасибо)) Внедрил библиотеки MapStruct, теперь код маппинга генерируется автоматически и работает быстрее.
| } | ||
|
|
||
| @GetMapping | ||
| public List<ItemDto> getAll(@RequestHeader("X-Sharer-User-Id") Long userId) { |
There was a problem hiding this comment.
Нужно вынести повторяющийся хэдер в отдельный класс и сделать его публичной константой, чтобы избежать дублирований в других классах. Чтобы быстро поправить все значения, можно воспользоваться горячими клавишами в IDEA: Ctrl+Shift+R
There was a problem hiding this comment.
Вывел повторяющийся хэдер в отдельный класс.
| import java.util.Map; | ||
| import java.util.Optional; | ||
|
|
||
| public abstract class AbstractRepository<T, K> { |
There was a problem hiding this comment.
Здорово, что создал абстрактный класс 👍
| @Override | ||
| public List<UserDto> getAll() { | ||
| List<UserDto> users = new ArrayList<>(); | ||
| for (User user : userRepository.findAll()) { |
There was a problem hiding this comment.
Улучшил код, используя Stream API.
|
|
||
| @SpringBootTest | ||
| @DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_EACH_TEST_METHOD) | ||
| class UserControllerTest { |
|
|
||
| private void throwIfEmailNotUnique(User user) { | ||
| for (User userCheck : userRepository.findAll()) { | ||
| if (user.getEmail().equals(userCheck.getEmail())) { |
There was a problem hiding this comment.
Дополнительные оптимизации
Чтобы проверка почты проходила за O(1), нужно завести Set, куда при добавлении нового пользователя мы будем класть его email. Соответственно, перед тем как добавить пользователя, нужно будет посмотреть, есть ли уже в сете подобный email, если есть - то пользователь с таким email уже существует.
Так как поиск и вставка в сете работают в среднем за O(1), то наш алгоритм поиска одинаковых email тоже будет работать в среднем за O(1), а это гораздо быстрее, чем пройтись по всем элементам списка O(n)
В коммерческой разработке уже давно сложно встретить обычные циклы, так как в java 8 был представлен более мощный инструмент для работы с коллекциями - stream api. С помощью стримов и лямбда - выражений работать с коллекциями гораздо удобнее и нативно понятнее, поэтому важно уметь их применять в своем коде
There was a problem hiding this comment.
Оптимизировал проверку уникальности email с помощью Set для достижения O(1) сложности.
1) Убрал лишние TODO. 2) Вывел повторяющийся хэдер в отдельный класс. 3) Улучшил код, используя Stream API. 4) Вынес метод Обновление полей в маппер, после внедрения MapStruct, генерируется автоматически. 5) Использовал stream api в методе getAll(). 6) Оптимизировал проверку уникальности email с помощью Set для достижения O(1) сложности. 7) Внедрил библиотеки MapStruct, теперь код маппинга генерируется автоматически и работает быстрее.
1) Убрал лишние TODO. 2) Вывел повторяющийся хэдер в отдельный класс. 3) Улучшил код, используя Stream API. 4) Вынес метод Обновление полей в маппер, после внедрения MapStruct, генерируется автоматически. 5) Использовал stream api в методе getAll(). 6) Оптимизировал проверку уникальности email с помощью Set для достижения O(1) сложности. 7) Внедрил библиотеки MapStruct, теперь код маппинга генерируется автоматически и работает быстрее. 8) Добавил пустые строки для прохождения проверки Checkstyle.
|
|
||
| public UserServiceImpl(UserRepository userRepository) { | ||
| this.userRepository = userRepository; | ||
| userRepository.findAll().forEach(user -> emailSet.add(user.getEmail().toLowerCase())); |
There was a problem hiding this comment.
Не совсем понимаю зачем тут предзаполнять данные по email. Какая была идея?
Кажется, когда ты создаешь экземпляр UserServiceImpl, данные по пользователю все равно будут пустыми, пока пользователь не начнет пользоваться нашим API. А так как у нас пока хранение inmemory, при перезапуске приложения все данные будут стерты. Вполне достаточно добавлять email в сет при создании пользователя
There was a problem hiding this comment.
Убрал лишний код из UserServiceImpl)
| import ru.practicum.shareit.user.dto.UserDto; | ||
| import ru.practicum.shareit.user.model.User; | ||
|
|
||
| public class UserMapper { |
There was a problem hiding this comment.
Здесь тоже можно использовать mapstruct
There was a problem hiding this comment.
Спасибо) Внедрил библиотеки MapStruct в UserMapper, теперь код стал более единообразным. Обновил тесты и конструктор.
1) Внедрил библиотеки MapStruct в UserMapper, теперь код стал более единообразным. 2) Обновил тесты и конструктор под MapStruct. 3) Убрал лишнее предзаполнение из UserServiceImpl.